New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename Retry options and defaults #2000
Conversation
a642692
to
43db004
Compare
# and if so we pass the deprecated 'method_whitelist' otherwise | ||
# we use 'method_allowlist'. Remove in v2.0 | ||
if "method_whitelist" not in kw and "method_allowlist" not in kw: | ||
if "method_whitelist" in self.__dict__: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm doing this instead of hasattr(self, "method_whitelist")
because that triggers a getattr()
call. Thanks Hynek for the blog post. If there's a better way, let me know :)
@@ -0,0 +1,428 @@ | |||
import mock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is literally a copy-paste of test_retry.py
with extra asserts about deprecated options. Done so that we can move along with test_retry.py
without having to think about deprecations much but also ensure that everything that used to work still does.
We'll remove this from master
once we cut a v1.26 branch
Codecov Report
@@ Coverage Diff @@
## master #2000 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 2187 2233 +46
=========================================
+ Hits 2187 2233 +46
Continue to review full report at Codecov.
|
d6676a9
to
ef831d4
Compare
ef831d4
to
77dd9d8
Compare
This is great, will feel good to purge this in v2.0 :) |
Thanks for review comments @sigmavirus24 and @shazow, rerequesting reviews from both of yall with the changes :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, only minor/optional suggestions. :)
src/urllib3/util/retry.py
Outdated
_Default = object() | ||
|
||
|
||
class RetryMeta(type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional nit: Maybe RetryDeprecations
or RetryWarnings
would be more descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to pass on this one since it's "Retry metaclass", maybe I should make the class private though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I just haven't written Python in too long, it has become weird to me to add type hinting into the name of things (rather than describe what they do).
Co-authored-by: Andrey Petrov <shazow@gmail.com>
Co-authored-by: Andrey Petrov <shazow@gmail.com>
Co-authored-by: Andrey Petrov <shazow@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started down a path of "There's got to be a better way than using a metaclass" for that and ended up in a dark dark place of Python I might have to turn into a library to make better.
@sigmavirus24 git init deprecatelib3 |
Tag line: break shit more often 🤘 |
DEPRECATE EVERYTHING / FEAR NOTHING! |
Thanks for the reviews @shazow and @sigmavirus24! |
Closes #1916
Still need to add some test cases on how setting both options interacts.